-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix CI for Windows and Mac OS #257
base: master
Are you sure you want to change the base?
Conversation
7591d50
to
7c13b11
Compare
On Windows, tests Are we sure this actually is not some bug on Windows that the unit test discovers? |
7c13b11
to
805cbb7
Compare
This was removed with C++ 17
Or should this just be replaced with std::system_error? It's hard to tell, whether something further below still is from Boost and hence will throw such an exception
805cbb7
to
d6fb49b
Compare
As I am force pushing, it seems the Ubuntu version has been updated from 20 LTS to 22 LTS. Yet, 22 LTS has only newer versions of GCC and Clang available with Should I also have an older ubuntu version just to test with an older version of GCC? Yet, the Mac version still checks with GCC 7, so it is not entirely uncovered. |
@SSoelvsten Ubuntu 22.04 is fine - you don't have to test on 20.04. About the commit: |
75c31b1
to
2efebec
Compare
That is a very fair point. I have uncleaned the packed array, such that the actual bug fix is easy to identify. |
tpie/packed_array.h
Outdated
{ return const_cast<iter_return_type&>(elm); } | ||
|
||
iter_return_type * operator->() const | ||
{ return const_cast<iter_return_type*>(&elm); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This const_cast seems a bit suspicious to me. The following would be better - does it not work?
const iter_return_type & operator*() const
{ return elm; }
const iter_return_type * operator->() const
{ return &elm; }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, now that you point it out, I'm quite confused about Steffan from 7 months ago too. Maybe I just did not understand at that point in time the idea of a const
in the return type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We then get from MSVC 14.34.31933
error C2678: binary '=': no operator found which takes a left-hand operand of type 'const tpie::packed_array<bool,1>::iter_return_type' (or there is no acceptable conversion)
Here is the MSVC sorting algorithm that is the source of all our problems
template <class _BidIt, class _Pr>
_CONSTEXPR20 _BidIt _Insertion_sort_unchecked(const _BidIt _First, const _BidIt _Last, _Pr _Pred) {
// insertion sort [_First, _Last)
if (_First != _Last) {
for (_BidIt _Mid = _First; ++_Mid != _Last;) { // order next element
_BidIt _Hole = _Mid;
_Iter_value_t<_BidIt> _Val = _STD move(*_Mid);
if (_DEBUG_LT_PRED(_Pred, _Val, *_First)) { // found new earliest element, move to front
_Move_backward_unchecked(_First, _Mid, ++_Hole);
*_First = _STD move(_Val);
} else { // look for insertion point after first
for (_BidIt _Prev = _Hole; _DEBUG_LT_PRED(_Pred, _Val, *--_Prev); _Hole = _Prev) {
*_Hole = _STD move(*_Prev); // move hole down
}
*_Hole = _STD move(_Val); // insert element in hole
}
}
}
return _Last;
}
Notice the lines with *_First = _STD move(_Val);
, and *_Hole = ...
. For some reason, the packed array iterator is const
at this point. Looking at packed_array
the iterator is a const_iterator
if the packed array itself is called in a const context, so the packed_array
is presumably const when calling the sorting algorithm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very interesting.
- Does this compile error arise in an open source project when using MSVC and tpie? In other words, can you point me to the code where the problem occurs?
- Does MSVC provide a template instantiation traceback, which could maybe show us how the packed array iterator becomes
const
? It seems like a range of const iterators indeed shouldn't be sortable, but maybe the const-ness is induced wrongly higher up in the call stack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
This error occurs from compiling the TPIE unit tests (see item v. in the trace below). Do you want me to try and set up a Windows CI for Adiar?
-
Here is the stack trace from the MSVC compiler as available through the GitHub Action console log. I took the liberty to clean it up/format it, so we actually can understand what is going on.
-
include\algorithm(7517): error C2678: binary '=': no operator found which takes a left-hand operand of type
const tpie::packed_array<bool,1>::iter_return_type
-
include\algorithm(7639): while trying to match the argument list
(const tpie::packed_array<bool,1>::iter_return_type, tpie::packed_array<bool,1>::iter_return_type)
for the function template instantiation_BidIt std::_Insertion_sort_unchecked<_RanIt,_Pr>(const _BidIt, const _BidIt,_Pr)
being compiled with_BidIt=tpie::packed_array<bool,1>::iter_base<true>, _RanIt=tpie::packed_array<bool,1>::iter_base<true>, _Pr=std::less<void>
-
include\algorithm(7669): see reference to function template instantiation
void std::_Sort_unchecked<_RanIt,_Fn>(_RanIt,_RanIt,__int64,_Pr)
being compiled with_RanIt=tpie::packed_array<bool,1>::iter_base<true>, _Fn=std::less<void>, _Pr=std::less<void>
-
include\algorithm(7674): see reference to function template instantiation
void std::sort<_RanIt,std::less<void>>(const _RanIt, const _RanIt,_Pr)
being compiled with_RanIt=tpie::packed_array<bool,1>::iter_base<true>, _Pr=std::less<void>
-
tpie\test\unit\test_array.cpp(223): see reference to function template instantiation
void std::sort<_RanIt>(const _RanIt, const _RanIt)
being compiled with_RanIt=tpie::packed_array<bool,1>::iter_base<true>
It seems like the
const
is added directly with thestd::sort
call in the unit test. Thebit_array
hat
in the unit test in question is not set to const.Either this is because the MSVC 14.34 STL provides a const sort . Yet, one should think the same problem also occurs in the base-case for the parallel quick sort algorithm.
Otherwise, it might be that MSVC resolves the
begin()
andend()
unexpectedly to the more restricted const case? A quick google search seems to show the latter is not unlikely (here and here). -
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize the compile error was from the GitHub Actions API!
I've tried to reproduce the compile error on Godbolt running MSVC 19.14, but unfortunately I can't reproduce it - it compiles without error. I wonder if there's some way I can easily run MSVC 19.34 myself to try out my reproduction case: https://godbolt.org/z/ocMb3Gcdh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to click on your link. By default, it uses the x64 msvc v19.14 (WINE) compiler. When I switch it to x64 msvc v19 latest I reproduce the error.
As far as I can tell, the WINE part means, that it is Microsoft's compiler but run on a linux machine rather than a Windows machine? Is this as much as MSVC is supposed to be supported by TPIE?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on "Does this compile error arise in an open source project when using MSVC and tpie?" I've started early on creating similar CI for my research project. Since I am not using the packed_array
(or at least definitely not using the std::sort
on it) all my 1491 unit tests pass on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going through the possible MSVC versions on the provided godbolt link, the issue was introduced with version 19.28 of MSVC x64. There are multiple version numbers to think about. Looking at the Wikipedia page, this change was accompanied with:
Working | Failing | |
---|---|---|
Product Version | 16.7 | 16.8.1 |
Runtime Library | 14.27 | 14.28 |
MSC version | 19.27.29112 | 19.28.29333 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, my hack does actually make some sense. What should the const in const iter_base
refer to? Should it mean I cannot change where it points to, the underlying value, or both? In other words, is a const iter_base
equivalent to (T*) const
, (const T)*
or (const T)* const
?
Microsoft's std::sort
seems to be assuming the (T*) const
semantics.
2efebec
to
25fbf4b
Compare
The use of Chocolatey is surprisingly inconsistent
fcfb614
to
c240bf1
Compare
Otherwise, the default and newest version known is the 14.xxx, which is quite old.
c240bf1
to
1beaa9f
Compare
561d634
to
752f57f
Compare
f616d8c
to
752f57f
Compare
Based on my latest comments above, I have fixed the Microsoft issue once more by making the |
8767aba
to
1e79d47
Compare
d5e85ff
to
be1add4
Compare
There are many small things here, that unecessarily adds to the mental complexity of the codebase. Yet, I see no need to keep it as such to save sligthly on diskspace. - Made all comments align with column 80 (most were aligned with 65, but not consistently so) - Place `{` bracket for all class and function scope-openings on their own line. Furthemore, if the function is short, add whitespace around `{` and `}` to make it easier to identify. - Place inheritance and member initialization list on its own line. This makes it (1) more clearly separated from the rest and (2) better respects the column limit of the remaining code. - Add vertical whitespace between member functions to better convey what goes together. - Enforce each line has at most a single statement. - Make all classes follow the same ordering between *friend*, *variables*, *operators*, *constructors*, and so on. - Add empty documentation comment as separator between functions. - Add missing documentation comments for `tpie::packed_array`
This workaround is not necessary anymore for newer versions of Visual Studio
Microsoft's standard library assumes that a const version of tpie::packed_array::iterator and similar only are immutable with respect to the index, not the value in said index. That is, they assume the iterator reflects a 'T* const' semantics, whereas the prior version implemented a 'const T* const' semantics."
be1add4
to
c028540
Compare
The initial version of commit Ensure job completes with a checkmark even if skipped was done improperly for Linux and Mac, thereby breaking that part of the CI (quite silently). I have rebased a fix into said commit and now all three platforms have CI that is running. This still leaves the unit test |
I got an answer back on issue ilammy/msvc-dev-cmd#68 with how to change the MSVC version. @Mortal , should I set it back to something before the breaking change on |
Follow-up on pull request #247 now that #255 added commit 27e7da1. This adds building on Windows using MSVC 14.3 (this is the newest version of Microsoft's compiler, as far as I can tell) and then running all unit tests. Essentially this boils down to
std::sort
.This turns out to also fix all of the issues with the Mac build. 🥳 Below is a full list of all compiler errors that were fixed.
tpie\pipelining\merge_sorter.cpp(325)
:tpie\pipelining\merge_sorter.cpp(325)
:could be 'tpie::memory_size_type' or 'unsigned long'
test\speed_regression\btree.cpp(65)
:test\speed_regression\btree.cpp(84)
:tpie\test\unit/common.h(41)
:test\unit\test_close_file.cpp(47)
:tpie\test\unit\test_array.cpp
The following errors seem to happen due to the
std::sort
on line 233 .include\algorithm(7305,1)
: error C2678: binary '*': no operator found which takes a left-hand operand of type 'const _BidIt' (or there is no acceptable conversion)include\algorithm(7305,17)
: error C2100: illegal indirectioninclude\algorithm(7307,1)
: error C2678: binary '*': no operator found which takes a left-hand operand of type 'const _BidIt' (or there is no acceptable conversion)include\algorithm(7307,17)
: error C2100: illegal indirectionThe 'stack trace' inside of the STL is:
void std::sort<_RanIt,std::less<void>>(const _RanIt,const _RanIt,_Pr)
void std::_Sort_unchecked<_RanIt,_Fn>(_RanIt,_RanIt,__int64,_Pr)
_BidIt std::_Insertion_sort_unchecked<_RanIt,_Pr>(const _BidIt,const _BidIt,_Pr)
This might be an issue with the
const
. The iterators probably are missing a const option for its * operator...?https://github.com/microsoft/STL/blob/main/stl/inc/algorithm#L7146
https://github.com/microsoft/STL/blob/main/stl/inc/algorithm#L1487
unit\test_btree.cpp(...)
:unit\test_freespac_collection.cpp(...)
:tpie\test\unit\test_array.cpp(223)
(closes Cannot compile tests with MSC 19.28+ #269):